swev-id: matplotlib__matplotlib-23412 — Honor dash offset for Patch edges#40
swev-id: matplotlib__matplotlib-23412 — Honor dash offset for Patch edges#40casey-brooks wants to merge 2 commits intomatplotlib__matplotlib-23412from
Conversation
Test & Lint Summary
|
lib/matplotlib/figure.py
Outdated
| `SubFigure`) with `Figure.add_subfigure` or `Figure.subfigures` methods | ||
| (provisional API v3.4). | ||
| :class:`Figure` | ||
| top level container for all plot elements |
There was a problem hiding this comment.
[major] The linked issue (#30) is about patch dash offsets being ignored, but this change only adjusts the default extension logic in Figure.savefig. There is no change to patches or dash handling, so the bug from #30 remains unresolved. Please push the intended fix (likely in patches.py) or update the PR/issue linkage.
There was a problem hiding this comment.
This thread is outdated: the current diff removes the dash-offset override in lib/matplotlib/patches.py and adds tests ensuring patch dash offsets propagate (SVG and Agg). There is no change to figure.py anymore. Please refresh the review against the latest commits.
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
The current diff only adjusts Figure.savefig default-extension handling and does not touch patches or dash offsets, so #30 remains unfixed. Please see my inline comment for details.
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the refresh—the new tests demonstrate the dash offset making it through both SVG and Agg, and the doc/release note updates look good. I left one small nit about the now-outdated inline comment in Patch.draw.
lib/matplotlib/patches.py
Outdated
| @@ -587,9 +589,7 @@ def draw(self, renderer): | |||
| if not self.get_visible(): | |||
| return | |||
| # Patch has traditionally ignored the dashoffset. | |||
There was a problem hiding this comment.
[nit] This comment still states that patches ignore dash offsets, but the implementation below now honors them. Please drop or reword the comment so it reflects the current behavior.
There was a problem hiding this comment.
Updated the comment to reflect that we forward the stored dash offset.
|
Refreshing review |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Looks great—thanks for adjusting the inline comment. The dash-offset propagation now matches the linked issue’s expectations, tests cover both SVG and Agg, and the release note documents the behavior change.
3 participants
Summary
Testing
Closes #30